refactor: split s7/ into standalone s7commplus/ package by gijzelaerr · Pull Request #774 · gijzelaerr/python-snap7

gijzelaerr · 2026-07-16T08:07:55Z

Summary

Implements #769 — splits the S7CommPlus protocol implementation out of the unified s7/ package into a standalone s7commplus/ package and deletes the auto-negotiating unified wrappers.

s7commplus/ package created with the S7CommPlus implementation renamed from internal modules to clean public names
s7/ package repurposed as an alias for snap7 — from s7 import Client is now the recommended way to use the legacy S7 protocol (replacing from snap7 import Client, which still works)
Users now explicitly choose s7.Client (legacy S7-300/400) or s7commplus.Client (S7CommPlus S7-1200/1500)

File moves

Old path	New path
`s7/_s7commplus_client.py`	`s7commplus/client.py`
`s7/_s7commplus_async_client.py`	`s7commplus/async_client.py`
`s7/_s7commplus_server.py`	`s7commplus/server.py`
`s7/{connection,protocol,codec,vlq,legitimation,typeinfo}.py`	`s7commplus/`

Deleted (unified wrappers)

s7/client.py — auto-negotiating Client with Protocol.AUTO
s7/async_client.py, s7/server.py — unified async client and dual-protocol server
s7/_protocol.py — Protocol enum (AUTO/LEGACY/S7COMMPLUS)
s7/partner.py, s7/util.py — pure re-exports of snap7 modules
tests/test_s7_unified.py — tested the deleted unified wrappers

Updated

10 test files — all from s7. → from s7commplus.
4 example files — renamed and rewritten for the new API
14 .rst doc files — updated to recommend s7 over snap7 for legacy protocol
Config: pyproject.toml, tox.ini, Makefile, CLAUDE.md, README.rst, CHANGES.md

Breaking changes (vs unreleased 4.0-dev only — 3.x is unaffected)

from s7 import Client now returns the legacy snap7.Client instead of the auto-negotiating S7CommPlus client — use from s7commplus import Client for S7CommPlus
Protocol enum removed — no more Protocol.AUTO/Protocol.S7COMMPLUS
Unified s7.Server removed — use s7commplus.Server or s7.server.Server directly

Test plan

1558 tests pass, 82 skipped
ruff check + format clean
mypy clean (pre-existing errors only)
pre-commit all hooks pass

🤖 Generated with Claude Code

gijzelaerr · 2026-07-16T08:15:33Z

@ale-rinaldi @Erik1000 @EliTheCoder @xBiggs — this PR splits the unified s7/ package into a standalone s7commplus/ package per #769. The auto-negotiating client that caused confusing fallback errors (#760, #710) is gone. Users now explicitly choose:

from snap7 import Client — legacy S7 (S7-300/400, PUT/GET)
from s7commplus import Client — S7CommPlus (S7-1200/1500)

There's also a new s7 alias that re-exports snap7 (so from s7 import Client still works for the legacy protocol — first step toward dropping the snap7 name).

Would appreciate your review on whether the new package structure makes sense and nothing got lost in the move. The PR description has the full file mapping.

Erik1000 · 2026-07-16T08:53:53Z

Guess it makes sense if the protocols differ so much and its definitely better than catching exceptions everywhere.

ale-rinaldi · 2026-07-16T09:46:57Z

Overall I'm in favour of the direction — a few thoughts, since you asked.

The split itself: 👍 Explicit s7commplus.Client vs snap7.Client and dropping the auto-negotiating wrapper is the right call. The silent fallback to legacy PUT/GET was a genuine footgun (Erik's #760, and #710) — good to see it gone.

The HarpoS7 session_auth import: this is really the bigger half of the PR, and I'd treat it separately from the restructure:

License is fine — HarpoS7 is MIT, same as here. But MIT still requires carrying the upstream copyright/notice; the transpiled files cite the source path but I'd add HarpoS7's LICENSE/copyright under session_auth/ (or a NOTICE) to be clean.
Two changes in one PR. A pure package move is reviewable at a glance; ~97k lines of auto-generated Family-0 monoliths are not. Splitting "restructure" and "session_auth import" into two PRs would make each reviewable — and let the crypto import get the focused scrutiny it deserves.
Reproducible verification. The files say "vector-verified against HarpoS7 test blobs" — are those blobs + tools/transpile_harpo_monolith.py wired into CI so the transpilation is re-verifiable? Otherwise the guarantee only held on the machine that generated them.
Wiring. legitimation.py doesn't reference session_auth; it'd help to note where Family-0 is actually used (V2 integrity? session-key derivation?) and whether all 47 modules are needed, so no dead weight ships.

One inconsistency worth a look — the s7 → snap7 alias. The PR's stated goal is "no more silent fallback to legacy," but aliasing s7 to snap7 means from s7 import Client keeps working while now returning the legacy client instead of the S7CommPlus one it used to — a silent protocol switch for exactly the existing s7 users, plus s7.Protocol disappears. That's the same class of surprise the PR is trying to remove. A clean ImportError/deprecation from s7 pointing at s7commplus/snap7 might be more consistent than silently handing back a different-protocol client. (For context, our probe reader is one of those s7.Client() + Protocol.S7COMMPLUS users — we'll migrate to s7commplus, no problem.)

Offer: the session-auth path is exactly what's still failing for us on real hardware (the legitimation challenge fetch in #718). We have an S7-1200 (1211C, FW4.6) and an S7-1500 (1507S, FW21.9) on hand — happy to run this branch against both and report whether legitimation/integrity actually completes end-to-end, which the emulated-server tests can't cover. Just say the word.

gijzelaerr · 2026-07-16T09:57:55Z

ah wait, i didn't realise the session auth stuff was also in this PR. going to take that out, it is not ready yet and quite a mess.

The unified s7/ package auto-negotiated between S7CommPlus and legacy S7, but the silent fallback created confusing errors (#760, #710). Split the S7CommPlus implementation into s7commplus/, delete the unified wrappers, and force users to explicitly choose snap7.Client (legacy S7-300/400) or s7commplus.Client (S7CommPlus S7-1200/1500). The s7/ package is retained as a snap7 alias for backward compat. File moves: - s7/_s7commplus_client.py → s7commplus/client.py - s7/_s7commplus_async_client.py → s7commplus/async_client.py - s7/_s7commplus_server.py → s7commplus/server.py - s7/{connection,protocol,codec,vlq,legitimation,typeinfo}.py → s7commplus/ Deleted unified wrappers: - s7/client.py, s7/async_client.py, s7/server.py, s7/_protocol.py - s7/partner.py, s7/util.py Updated: all tests, examples, docs, pyproject.toml, tox.ini, Makefile. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

…-split-v2

Update docs, README, CLAUDE.md, and examples to use `from s7 import` as the recommended import for the legacy S7 protocol. The `snap7` name continues to work for backwards compatibility but is no longer the primary recommendation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

gijzelaerr · 2026-07-16T11:11:15Z

Thanks both for the feedback.

@Erik1000 — agreed, explicit protocol choice is much better than catching surprises.

@ale-rinaldi — good review, addressing each point:

session_auth: Removed from this PR entirely — it was accidentally included via the base branch. It'll come as a separate PR with proper license attribution, CI-wired transpilation verification, and documentation on where Family-0 plugs in. Your notes on all of those are spot-on and will be addressed there.

The s7 alias concern: Fair point, but since s7 was never in a released version (only existed in unreleased 4.0-dev), there are no real-world s7.Client() + Protocol.S7COMMPLUS users to surprise. We've decided to make s7 the recommended name for the legacy protocol going forward — from s7 import Client is now the primary recommendation in all docs, with snap7 kept as an undeprecated backwards-compatible alias. So the naming is now:

from s7 import Client — legacy S7 (recommended)
from snap7 import Client — same thing, still works
from s7commplus import Client — S7CommPlus

No cross-wiring between s7 and s7commplus.

Hardware testing offer: Would love that — let's pick it up on the session_auth PR once it's ready. Having real S7-1200 FW4.6 + S7-1500 FW21.9 coverage would be invaluable for the legitimation path.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

autoweave-bot · 2026-07-16T12:44:38Z

This looks like a great step towards a more robust and secure S7CommPlus setup! 🚀😸
Explore here →

_{Maintainer? Turn off weaves from non-maintainers →}
_{Carefully crafted by Subweave · 🧶 used ~618k LLM tokens}

Rebased onto the s7commplus/ package split (#774). All session_auth code now lives under s7commplus/session_auth/ instead of s7/session_auth/. Adds the full session_auth package for V1-initial S7-1200 PLCs that require a SessionKey handshake before data operations: - s7commplus/session_auth/: Public-key-based session authentication (Family 0 key derivation, fingerprint matching, SecurityKeyEncryptedKey blob generation, legitimation challenge solver, HMAC key derivation) - V3 HMAC framing for post-auth data operations - TIA-style CreateObject attributes (required by V1-initial firmware) - PAOM string stripping in ServerSessionVersion echo - Post-auth legitimation flow (challenge read + solved blob write) - Connection.connect(password=) parameter for auth flow Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

github-advanced-security AI found potential problems Jul 16, 2026

View reviewed changes

Comment thread s7commplus/session_auth/family0/authenticator.py Fixed

Comment thread s7commplus/session_auth/family0/authenticator.py Fixed

Comment thread s7commplus/session_auth/harpo_aes.py Fixed

Comment thread s7commplus/session_auth/legitimate.py Fixed

gijzelaerr force-pushed the refactor/s7commplus-split branch from 37e21f0 to ed71eb1 Compare July 16, 2026 10:56

gijzelaerr and others added 2 commits July 16, 2026 12:57

Merge remote-tracking branch 'origin/master' into refactor/s7commplus…

3a1929d

…-split-v2

gijzelaerr and others added 2 commits July 16, 2026 13:22

fix: regenerate uv.lock with public PyPI URLs

8a426d5

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

fix: regenerate uv.lock with public PyPI URLs

aff3adf

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

gijzelaerr merged commit e4d5396 into master Jul 16, 2026
39 checks passed

This was referenced Jul 16, 2026

SessionKey handshake for V1-initial S7-1200 PLCs #761

Closed

SessionKey handshake for V1-initial S7-1200 PLCs #775

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor: split s7/ into standalone s7commplus/ package#774

refactor: split s7/ into standalone s7commplus/ package#774
gijzelaerr merged 5 commits into
masterfrom
refactor/s7commplus-split

gijzelaerr commented Jul 16, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gijzelaerr commented Jul 16, 2026

Uh oh!

Erik1000 commented Jul 16, 2026

Uh oh!

ale-rinaldi commented Jul 16, 2026

Uh oh!

gijzelaerr commented Jul 16, 2026

Uh oh!

gijzelaerr commented Jul 16, 2026

Uh oh!

autoweave-bot commented Jul 16, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

gijzelaerr commented Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

File moves

Deleted (unified wrappers)

Updated

Breaking changes (vs unreleased 4.0-dev only — 3.x is unaffected)

Test plan

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gijzelaerr commented Jul 16, 2026

Uh oh!

Erik1000 commented Jul 16, 2026

Uh oh!

ale-rinaldi commented Jul 16, 2026

Uh oh!

gijzelaerr commented Jul 16, 2026

Uh oh!

gijzelaerr commented Jul 16, 2026

Uh oh!

autoweave-bot commented Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gijzelaerr commented Jul 16, 2026 •

edited

Loading

autoweave-bot commented Jul 16, 2026 •

edited

Loading